[#12434] Add Node Interpolation#12506
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12506 +/- ##
=========================================
Coverage 33.58% 33.58%
- Complexity 10744 10746 +2
=========================================
Files 3941 3941
Lines 91887 91889 +2
Branches 9558 9558
=========================================
+ Hits 30857 30859 +2
- Misses 58387 58388 +1
+ Partials 2643 2642 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors MapServiceImpl to improve readability and modularity by extracting special-case logic, simplifying factory method signatures, and cleaning up imports.
- Introduces
buildForNoRequestto handle empty-node scenarios inselectApplicationMap - Simplifies
newNodeHistogramFactoryandnewServerGroupListFactoryto take boolean flags instead of the fullMapServiceOption - Updates
selectApplicationMapto delegate to the new method for no-request cases
Comments suppressed due to low confidence (3)
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/MapServiceImpl.java:130
- No new tests cover the
buildForNoRequestpath; consider adding a unit test to verify that an empty-node scenario produces the expected map with only the source application.
map = buildForNoRequest(option.getSourceApplication(), builder);
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/MapServiceImpl.java:142
- [nitpick] The parameter order
(Application source, ApplicationMapBuilder builder)is inverted compared to other helpers; consider placing the builder parameter first for consistency with existing methods.
private ApplicationMap buildForNoRequest(Application source, ApplicationMapBuilder builder) {
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/MapServiceImpl.java:144
- Since
includeServerInfois already called increateApplicationMapBuilder, this second call may add duplicate server info. Verify that re-inclusion is intended or clear previous server info before adding the fallback.
builder.includeServerInfo(newServerGroupListFactory(false));
|
|
||
| private ApplicationMap buildForNoRequest(Application source, ApplicationMapBuilder builder) { | ||
| // If no nodes are found, we still need to build the map with the source application. | ||
| builder.includeServerInfo(newServerGroupListFactory(false)); |
There was a problem hiding this comment.
[nitpick] Using a raw false literal makes the intent unclear; extract this flag into a well-named constant or consider overloading newServerGroupListFactory for readability.
| builder.includeServerInfo(newServerGroupListFactory(false)); | |
| builder.includeServerInfo(newServerGroupListFactory(USE_STATISTICS_AGENT_STATE_FOR_NO_REQUEST)); |
| @@ -138,19 +139,25 @@ public ApplicationMap selectApplicationMap(MapServiceOption option) { | |||
| return map; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Consider adding a Javadoc comment to explain the purpose of buildForNoRequest and describe its parameters, since it encapsulates special-case behavior.
| /** | |
| * Builds an {@link ApplicationMap} for scenarios where no request data is available. | |
| * This method ensures that the map is still constructed using the source application, | |
| * even if no nodes are found. | |
| * | |
| * @param source The source application for which the map is being built. | |
| * @param builder The {@link ApplicationMapBuilder} used to construct the map. | |
| * @return The constructed {@link ApplicationMap}. | |
| */ |
This pull request refactors the
MapServiceImplclass inweb/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/MapServiceImpl.javato improve code readability and modularity. Key changes include introducing a new method for handling cases with no request data, simplifying method signatures, and modifying factory methods to accept boolean parameters instead ofMapServiceOption.Refactoring for improved modularity:
buildForNoRequestmethod: Added a private method to handle scenarios where no nodes are found in the application map, ensuring the map is still built with the source application. This improves readability and encapsulates logic.Simplification of method signatures:
newNodeHistogramFactorymethod: Changed the method to accept a boolean parameter (isSimpleResponseHistogram) instead of the entireMapServiceOptionobject, simplifying its usage.newServerGroupListFactorymethod: Updated the method to accept a boolean parameter (isUseStatisticsAgentState) instead ofMapServiceOption, aligning with the changes made tonewNodeHistogramFactory.Code cleanup and minor adjustments:
selectApplicationMaplogic: Replaced direct calls tobuilder.buildwith the newbuildForNoRequestmethod for handling empty nodes, improving clarity.Application: Included the necessary import for theApplicationclass to support the newbuildForNoRequestmethod.